-
Notifications
You must be signed in to change notification settings - Fork 21
feat: Add multiarch support to csi-snapshot-metadata Dockerfile #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @kaovilai. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
CSI Snapshot Metadata Container Readiness Probe Fails on ARM64Problem:
Impact: Low - Container is functional despite readiness probe failure; CBT APIs work correctly Evidence: $ kubectl describe pod csi-hostpathplugin-0 -n default
Warning Unhealthy Readiness probe failed: exec: Exec format errorRoot Cause: |
Refactors the Dockerfile to build the csi-snapshot-metadata binary from source in the builder stage, similar to how grpc_health_probe is obtained (via ADD from remote URL). Changes to cmd/csi-snapshot-metadata/Dockerfile: - Use golang:1.24-alpine as builder base (was alpine) - Add TARGETOS and LDFLAGS build arguments - Copy Go source files (go.mod, go.sum, cmd/, pkg/, client/, vendor/) - Build binary in builder with cross-compilation support - Copy built binary from builder (was copying from host filesystem) - Remove obsolete binary ARG Changes to release-tools/build.make: - Replace --build-arg binary with --build-arg LDFLAGS - Remove build-% dependency from push-multiarch-% target (Docker now builds from source, eliminating duplicate compilation) - Fix manifest error detection to handle both error formats: - "manifest for ... not found" (docker.io) - "manifest unknown" (ghcr.io, quay.io) Benefits: - Self-contained builds (no external Makefile dependency for Docker) - True cross-compilation via Docker buildx - Consistent approach (both binaries prepared in builder stage) - Easier for contributors (just docker/podman build) Tested with multiarch build to ghcr.io (amd64, arm64). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
0fb6f90 to
0e03558
Compare
|
/ok-to-test |
|
@kaovilai: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
The released snapshot-metadata sidecar images are already multiarch, built by Makefile in this repository. What exactly are you trying to achieve? |
No, csi-snapshot-metadata:canary is available as s390x, ppc64le, arm64, arm and amd64 |
|
@jsafrane I want to remove hardcoded amd64 for one from the Dockerfile.
The main change per pr description is
The rest is to speed up build make it possible to build anywhere with just docker/podman build. Making all bin builds happen inside the container instead of outside. |
|
Without my change health probe WILL FAIL via exec format error on non amd64 cluster. I know cause I ran it from said "already multiarch" image. |
|
|
Deps management are much cleaner and more easily pluggable into CI systems or other container build automation (such as quay.io auto build on push, where build env do not know anything about makefile crossbuild bins) if all the requirements are done in a container instead of cross-build in Makefile then copy-built-bins-into-container. |
|
Oh, I see, this image includes a binary from alpine! That's bad, please remove it. Such binary does not go through any CVE scanning. We in Kubernetes make a huge effort not to include any 3rd party binaries in our images that we don't control. Try to use a different health probe - e.g. a simple web server with BTW, it would be better not to include huge wall of AI generated text and write something significantly shorter where information does not get lost. |
|
Wtal. Thanks |
Do you mean Would |
|
Do not use any builder image. |
|
Even if its a multi-stage build? the build image isn't included in final container image, only the built binary on current distroless. |
|
How can I trust k8s buildfarm or local build environment any more than the (imo more verifiable) build image? |
Why do you need a multi-stage build? This sidecar is a super simple go binary.
There is a Kubernetes SIG-testinfra and SIG-release teams that keep the build farm up to date. If you want to challenge their skills or competences, please go to the corresponding teams. By using a builder image as alpine, the binary will be compiled by a different go version than is used for unit tests and even e2e test. |
It was already multi-stage prior to my PR. It also solves following.
Assuming version is the go module directive, or toolchain directive, we can easily reuse values from there here. So summarizing the requested changes:
|
Summary
This PR refactors the Dockerfile to enable multiarch support and build the csi-snapshot-metadata binary from source in the builder stage.
Changes
1. Multiarch grpc_health_probe support
grpc_health_probe-linux-${TARGETARCH}instead of hardcoded amd64--platform=$BUILDPLATFORMto builder stage for cross-compilation2. Build csi-snapshot-metadata in builder stage
Dockerfile changes:
golang:1.24-alpineas builder base (wasalpine)binaryARGMakefile changes (release-tools/build.make):
--build-arg binarywith--build-arg LDFLAGSbuild-%dependency frompush-multiarch-%target(Docker now builds from source, eliminating duplicate compilation)
Benefits
docker buildorpodman build)Testing
Tested with multiarch build to ghcr.io:
make push-multiarch-csi-snapshot-metadata \ REGISTRY_NAME=ghcr.io/kaovilai \ PULL_BASE_REF=test-20251119-014146 \ BUILD_PLATFORMS="linux amd64 amd64; linux arm64 arm64"Verified multiarch manifest contains both architectures:
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
Enables building multiarch container images (amd64, arm64) with the csi-snapshot-metadata binary built from source in the Dockerfile builder stage, making the build process self-contained and easier for contributors.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
make build-csi-snapshot-metadatastill works for local developmentDoes this PR introduce a user-facing change?: